Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #36688 - Provide option to use wget for the new Register Host feature #9808

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

goarsna
Copy link
Contributor

@goarsna goarsna commented Aug 21, 2023

The Register hosts page uses curl for registering hosts. This seems to be a good approach for EL based systems as most of these come with curl preinstalled. On Debian based systems this is not always the case (at least for recent versions of Debian and Ubuntu).

This feature request aims to provide an option to use wget for registering hosts as wget is as widely used as curl. By this system administrators that only have wget installed are not forced to also install curl anymore.

@theforeman-bot
Copy link
Member

Can one of the admins verify this patch?

2 similar comments
@theforeman-bot
Copy link
Member

Can one of the admins verify this patch?

@theforeman-bot
Copy link
Member

Can one of the admins verify this patch?

@theforeman-bot
Copy link
Member

Issues: #36688

@goarsna
Copy link
Contributor Author

goarsna commented Aug 21, 2023

I have opened a PR to document this in foreman-documentation:
theforeman/foreman-documentation#2376

@goarsna
Copy link
Contributor Author

goarsna commented Sep 4, 2023

Thanks @ekohl. I've incorporated your feedback and rebased my branch.

@stejskalleos
Copy link
Contributor

ok to test

@sbernhard
Copy link
Contributor

sbernhard commented Jul 25, 2024

The failed tests are not related.

Any objections to get this change in @stejskalleos / @ekohl / @ofedoren ?

sbernhard
sbernhard previously approved these changes Jul 25, 2024
Copy link
Contributor

@sbernhard sbernhard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @goarsna

Copy link
Member

@ofedoren ofedoren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly minor issues:

@ofedoren
Copy link
Member

Some js tests failed, could you please run npm run test -- -u to regenerate the snapshots?

@goarsna
Copy link
Contributor Author

goarsna commented Jul 29, 2024

Hey @ofedoren, thanks again for your review.
I left one comment above and added missing Apipie documentation for the download_utility in the registration_controller.rb 🙈

command << utility[:output_pipe]
end
headers&.each { |header| command << header }
utility[:format_params].call(params).each { |param| command << param } if params
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to fix this because it generated (in case of curl):
`--data "packages=Paket1\ Paket2" which would mean it tries to install a package with the name "Paket1 Paket2" (which does not work because 2 packages are meant")

Copy link
Contributor Author

@goarsna goarsna Aug 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Bernhard, I removed the quotes around the post parameters. This made it necessary to escape the ampersands in the wget post parameter.
In my tests installing multiple packages during host registration now worked as expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ofedoren Any thoughts about the solution with removing the quotes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I should provide some additional information 😅

Post parameters have to be provided differently for curl (one --data argument for each post parameter) and wget (one --post-data argument containing all post parameters). Initially I removed the quotes of the post parameters in the Global Registration template to unify them and when formatting the post parameters for the curl or wget command respectively I added quotes around each post parameter in the case of curl and around the whole --post-data argument in the case of wget. But by this I added quotes also to those post parameters that where not quoted before (like the packages post parameter) which is causing the described problem.
While investigating the problem with installing multiple packages, I was thinking about whether it even makes sense to quote any of the post parameters as the parameters that could cause problems are already escaped. So my new approach is to not quote the parameters and escape them correctly.

Copy link
Contributor

@sbernhard sbernhard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--data "packages=Paket1\ Paket2" <- this need to be fixed. see comment above

@goarsna
Copy link
Contributor Author

goarsna commented Sep 2, 2024

I talked with @sbernhard and we were wondering why there are no snapshot tests for the Global Registration template. I am currently working on this and added an initial approach.

@goarsna
Copy link
Contributor Author

goarsna commented Sep 2, 2024

Rebased

@goarsna
Copy link
Contributor Author

goarsna commented Sep 5, 2024

Ok, I tried to add sensible and reasonable snapshot tests for the Global Registration template with various parameters set. But this consumes more time than expected.
As I do not want to block this PR with it, I'll move adding of snapshot tests for the Global Registration template to a separate PR.

How do we want to proceed with this PR? I hope it can be merged soon. :)

Copy link
Contributor

@sbernhard sbernhard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@sbernhard
Copy link
Contributor

@ekohl / @stejskalleos you requested changes. Can you please check again. Thanks.

@maximiliankolb
Copy link
Contributor

@stejskalleos Friendly reminder to please re-review this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants